Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wip; Store some client settings #1167

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from
Draft

Conversation

PIG208
Copy link
Member

@PIG208 PIG208 commented Dec 17, 2024

Work towards #97.

@PIG208 PIG208 force-pushed the pr-settings branch 2 times, most recently from d5c014e to 370a328 Compare December 17, 2024 00:28
This has been addressed in Greg's upstream PR:
  https://git hub.com/simo lus3/drift/pull/3022

Signed-off-by: Zixuan James Li <[email protected]>
Signed-off-by: Zixuan James Li <[email protected]>
Signed-off-by: Zixuan James Li <[email protected]>
These were supposed to be updated in commit
fc80be1
where we replaced loadGlobalStore with getGlobalStore.

`LiveGlobalBindings.load` is the only thing left analogous to the
previously referred to `loadGlobalStore` method. To avoid direct
referencce to this specific implementation on `GlobalStore`, mention
that loading it for the first time is expected to be asynchronous
instead.

Signed-off-by: Zixuan James Li <[email protected]>
Signed-off-by: Zixuan James Li <[email protected]>
Also need tests for the global store change.

And UI for updating the settings.

Signed-off-by: Zixuan James Li <[email protected]>
@gnprice
Copy link
Member

gnprice commented Dec 17, 2024

Thanks for taking this on! Looking forward to it.

One first high-level comment: in the branch we merge, let's have at least one of these two settings get added as its own separate commit, complete with a migration. That way we have a nice demonstration of what it'll look like to add each new setting in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants